Skip to content

2025 08 11 int - #397

Merged
thedavidmeister merged 13 commits into
mainfrom
2025-08-11-int
Aug 12, 2025
Merged

2025 08 11 int#397
thedavidmeister merged 13 commits into
mainfrom
2025-08-11-int

Conversation

@thedavidmeister

@thedavidmeister thedavidmeister commented Aug 11, 2025

Copy link
Copy Markdown
Contributor

Motivation

Solution

Checks

By submitting this for review, I'm confirming I've done the following:

  • made this PR as small as possible
  • unit-tested any new functionality
  • linked any relevant issues or PRs
  • included screenshots (if this involves a front-end change)

Summary by CodeRabbit

  • New Features

    • Re-enabled the Ceiling (ceil) operation with decimal floating‑point semantics and an expanded user-facing description.
  • Behavior Changes

    • Headroom computation updated to use ceiling-based logic, which may alter headroom outputs.
  • Tests

    • Added executable Ceil tests and broadly updated math tests to normalized decimal floating‑point encodings and expectations.
  • Chores

    • Updated an internal interpreter submodule reference; no functional or API changes.

@coderabbitai

coderabbitai Bot commented Aug 11, 2025

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

Walkthrough

Re-enabled the Ceil opcode and wired it into the standard-op registry; migrated LibOpCeil and its tests to OperandV2/Float/StackItem using LibDecimalFloat; bumped the rain.interpreter.interface submodule pointer; adjusted many math-test expected float encodings and updated LibOpHeadroom runtime to use a.ceil() - a.

Changes

Cohort / File(s) Summary
Submodule bump
lib/rain.interpreter.interface
Updated submodule commit pointer; no API or runtime behavior changes beyond the pointer.
Standard ops registry
src/lib/op/LibAllStandardOps.sol
Re-enabled Ceil: activated import, incremented ALL_STANDARD_OPS_LENGTH (71→72), added Ceil authoring metadata and inserted operand-handler / integrity / run pointers.
Ceil opcode implementation
src/lib/op/math/LibOpCeil.sol
Migrated to OperandV2/StackItem and decimal Float (LibDecimalFloat); updated signatures (integrity, run, referenceFn) and implemented runtime/reference logic using Float.ceil().
Ceil tests
test/src/lib/op/math/LibOpCeil.t.sol
New LibOpCeilTest using OperandV2, Float, StackItem, and LibDecimalFloat; added integrity/run/eval tests and operand-disallowed checks.
Headroom runtime change
src/lib/op/math/LibOpHeadroom.sol
Replaced headroom computation with a.ceil() - a and clamp-to-1 if zero; applied in run and referenceFn. No signature changes.
Math test expectation updates
test/src/lib/op/math/LibOpGm.t.sol, test/src/lib/op/math/LibOpHeadroom.t.sol, test/src/lib/op/math/LibOpSub.t.sol, test/src/lib/op/math/growth/LibOpLinearGrowth.t.sol, test/src/concrete/RainterpreterReferenceExtern.intInc.t.sol, test/src/lib/op/math/LibOpAdd.t.sol, test/src/lib/op/math/LibOpExp.t.sol
Normalized decimal-float encodings and updated expected packed mantissa/exponent values in many tests to match LibDecimalFloat/Float packing semantics; test logic otherwise preserved (some test harness refactors).

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Interpreter
  participant LibAllStandardOps
  participant LibOpCeil
  participant LibDecimalFloat

  Caller->>Interpreter: execute program with Ceil opcode
  Interpreter->>LibAllStandardOps: resolve Ceil -> operand/integrity/run pointers
  LibAllStandardOps-->>Interpreter: return pointers
  Interpreter->>LibOpCeil: run(state, operandV2, stackTop)
  LibOpCeil->>LibDecimalFloat: Float.ceil() on stack value
  LibDecimalFloat-->>LibOpCeil: ceiled Float
  LibOpCeil-->>Interpreter: write result to stack, return new stackTop
  Interpreter-->>Caller: return final result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Possibly related PRs

  • 2025 08 07 gm #396: Broad refactor migrating ops from fixed-point to LibDecimalFloat/Float and updating signatures/pointers — closely matches Ceil migration.
  • 2025 07 09 div #351: Migration of op implementations and tests to OperandV2/Float/StackItem patterns — strong overlap.
  • 2025 07 31 const #388: Modifies LibAllStandardOps.sol (ALL_STANDARD_OPS_LENGTH and op pointer arrays) — related registry changes.

Suggested reviewers

  • findolor
  • hardyjosh

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6bc106a and 8f45075.

📒 Files selected for processing (1)
  • test/src/lib/op/math/LibOpCeil.t.sol (1 hunks)
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 2025-08-11-int

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6f7bc6 and 40777df.

⛔ Files ignored due to path filters (4)
  • src/generated/Rainterpreter.pointers.sol is excluded by !**/generated/**
  • src/generated/RainterpreterExpressionDeployer.pointers.sol is excluded by !**/generated/**
  • src/generated/RainterpreterParser.pointers.sol is excluded by !**/generated/**
  • src/generated/RainterpreterReferenceExtern.pointers.sol is excluded by !**/generated/**
📒 Files selected for processing (4)
  • lib/rain.interpreter.interface (1 hunks)
  • src/lib/op/LibAllStandardOps.sol (6 hunks)
  • src/lib/op/math/LibOpCeil.sol (1 hunks)
  • test/src/lib/op/math/LibOpCeil.t.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: git-clean
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
  • GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
🔇 Additional comments (7)
lib/rain.interpreter.interface (2)

1-1: LGTM: submodule pointer bump only

Pointer updated to a new commit; no direct code surface here.


1-1: Verify submodule commit and CI checkout settings

The lib/rain.interpreter.interface path is a Git submodule (a directory), not a pointer file. Before merging, please manually confirm:

  • Extract the recorded SHA
    git submodule status lib/rain.interpreter.interface | awk '{print $1}'
  • Confirm the commit exists upstream
    OWNER=$(git config -f .gitmodules submodule.lib/rain.interpreter.interface.url \
              | sed -E 's@(.*/)([^/]+)\.git@\1\2@')
    SHA=$(git submodule status lib/rain.interpreter.interface | awk '{print $1}')
    gh api "repos/${OWNER}/commits/${SHA}" -q .sha >/dev/null
  • Ensure your CI pulls submodules
    In your .github/workflows/*.yml, verify you use:
    - uses: actions/checkout@v4
      with:
        submodules: true  # or recursive: true

Please run these steps to verify the SHA and CI configuration before approving.

src/lib/op/math/LibOpCeil.sol (1)

23-34: Unary math ops consistently use stackTop as the top-item pointer

All inspected unary math operations load and store directly at stackTop (no instance of sub(stackTop, 0x20)), including LibOpCeil.sol. The convention is that stackTop already points at the input slot, so reading and writing at stackTop is correct and consistent.

No changes required here.

src/lib/op/LibAllStandardOps.sol (2)

75-75: Ceil import enabled

Re-enabling the import is correct and aligns with adding Ceil back into the standard set.


273-274: Ceil wired into authoring metadata, integrity, and runtime

  • Authoring metadata description is clear.
  • Operand handling correctly disallows operands for ceil.
  • Integrity and opcode function pointer insertions are in the correct alphabetical position.

LGTM.

Also applies to: 604-605, 715-716

test/src/lib/op/math/LibOpCeil.t.sol (2)

20-28: Solid reference vs runtime equivalence check

opReferenceCheck usage with OperandV2 and StackItem[] is aligned with the new API. Good direct coverage for run vs referenceFn.


56-59: Operand disallowed path covered

Good to keep the parse unhappy-path for unexpected operand.

Comment thread src/lib/op/LibAllStandardOps.sol
Comment thread src/lib/op/math/LibOpCeil.sol
Comment thread test/src/lib/op/math/LibOpCeil.t.sol

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40777df and 11059d8.

📒 Files selected for processing (5)
  • lib/rain.interpreter.interface (1 hunks)
  • test/src/lib/op/math/LibOpGm.t.sol (1 hunks)
  • test/src/lib/op/math/LibOpHeadroom.t.sol (1 hunks)
  • test/src/lib/op/math/LibOpSub.t.sol (1 hunks)
  • test/src/lib/op/math/growth/LibOpLinearGrowth.t.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
  • GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
  • GitHub Check: git-clean
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
🔇 Additional comments (12)
test/src/lib/op/math/LibOpGm.t.sol (1)

58-58: LGTM! Improved test data encoding maintains mathematical correctness.

The updated test expectation uses the new decimal-float encoding format (2.8285e66, -66) which is more readable than the previous representation (282850000000000000000000000000000000000000, -41). Both values are mathematically equivalent and correctly represent the geometric mean of 2 and 4 (√8 ≈ 2.8285).

lib/rain.interpreter.interface (1)

1-1: Confirm submodule bump and verify no breaking changes

All automated checks passed but the API surface diff wasn’t shown—please double-check that nothing unexpected slipped in:

• .gitmodules (path/url) is correct for lib/rain.interpreter.interface
• Gitlink updated from 0c8a593… → 78a0e52 (matches what’s recorded in the tree)
• CI must include something like:

git submodule sync --recursive
git submodule update --init --recursive --depth=1

Run this to review the changelog and diffstat between the old and new commits:

git -C lib/rain.interpreter.interface fetch origin 0c8a593691cec60714d87887cee0038b908b226f
echo "== Commit log =="
git -C lib/rain.interpreter.interface log --oneline --no-merges 0c8a593691cec60714d87887cee0038b908b226f..78a0e5226f78d69fcb0848e57a255b56fdaaf6f2
echo
echo "== Diffstat =="
git -C lib/rain.interpreter.interface diff --stat 0c8a593691cec60714d87887cee0038b908b226f 78a0e5226f78d69fcb0848e57a255b56fdaaf6f2

If you see any unintended API or behavior changes, update accordingly.

test/src/lib/op/math/growth/LibOpLinearGrowth.t.sol (6)

57-58: Numeric encoding normalized to e67/-67; values are unchanged

The new coefficients/exponents still encode 1.1 and 1.2 exactly; both fit safely within int224 (well below ~1.7e67). Good normalization.


60-60: Exact decimal preserved (1.25) with new scale

1.25e67 at -67 remains 1.25 exactly. Safe for int224 and consistent with the suite-wide shift.


64-64: Half-step case remains exact with new representation

1.05e67 at -67 encodes 1.05 exactly. Nice.


67-68: Two-input “2 + 0.1*t” expectations correctly normalized

2.1e66/2.2e66 at -66 encode 2.1/2.2 precisely. Fits int224 margins with room to spare.


70-70: Negative rate case normalized; magnitude within bounds

0.9e67 at -67 encodes 0.9. Coefficient within safe int224 range.


72-72: Negative a and r case normalized; still exact

-1.2e67 at -67 encodes -1.2 exactly; well within int224 min bound.

test/src/lib/op/math/LibOpHeadroom.t.sol (2)

34-34: 0.5 expectation normalized (5e66,-67) → 0.5

Exact decimal preserved; consistent with other tests moving to -67 exponent scale.


37-37: 3.8 headroom expectation normalized (2e66,-67) → 0.2

Encodes 0.2 exactly under the new scheme. Looks good.

test/src/lib/op/math/LibOpSub.t.sol (2)

106-108: Mixed-scale expectations normalized; values unchanged

  • 1 − 2 → -1 encoded as (-1e67, -67)
  • 1 − 0.1 → 0.9 encoded as (9e66, -67)
    Both are exact under decimal float.

111-115: Max-value edge case retained; label clarified; zero case added

Keeping max-positive-value() − 1 saturating back to max under float semantics makes sense. The added equal-max case asserting zero is a good guard.

Comment thread test/src/lib/op/math/growth/LibOpLinearGrowth.t.sol
Comment thread test/src/lib/op/math/LibOpHeadroom.t.sol
Comment thread test/src/lib/op/math/LibOpSub.t.sol
Comment thread test/src/lib/op/math/LibOpSub.t.sol
Comment thread test/src/lib/op/math/LibOpSub.t.sol
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/lib/op/math/LibOpCeil.sol (1)

16-19: Integrity: correct arity (1 in, 1 out) and grammar fixed

Return values align with a unary op. The comment now reads “one input and one output” as previously requested.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 11059d8 and 65e3d81.

📒 Files selected for processing (1)
  • src/lib/op/math/LibOpCeil.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: git-clean
  • GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
🔇 Additional comments (2)
src/lib/op/math/LibOpCeil.sol (2)

11-14: Library scaffolding and using directive look good

Natspec and the LibDecimalFloat extension setup are clear and correct.


21-34: No action necessary: assembly("memory-safe") is supported by your pinned compiler

The project’s foundry.toml pins solc = "0.8.25", which is ≥ 0.8.22 and fully supports the "memory-safe" assembly flag. No fallback or pragma changes are needed.

  • foundry.toml: solc = "0.8.25"

Comment thread src/lib/op/math/LibOpCeil.sol Outdated
Comment thread src/lib/op/math/LibOpCeil.sol

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (6)
test/src/lib/op/math/LibOpSub.t.sol (3)

78-80: Unify zero representation to literal 0 for readability

Zero is represented as a packed float here; elsewhere in this file a plain literal 0 is used. Prefer a consistent literal to reduce noise and aid readability.

Apply this diff:

-        checkHappy("_: sub(1 1);", Float.unwrap(LibDecimalFloat.packLossless(0, -75)), "1 1");
+        checkHappy("_: sub(1 1);", 0, "1 1");
         checkHappy("_: sub(2 1);", Float.unwrap(LibDecimalFloat.packLossless(1e67, -67)), "2 1");
-        checkHappy("_: sub(2 2);", Float.unwrap(LibDecimalFloat.packLossless(0, -75)), "2 2");
+        checkHappy("_: sub(2 2);", 0, "2 2");

106-106: Three-input zero result: unify zero representation

Match the rest of the file by using a plain 0 literal for zero results.

Apply this diff:

-        checkHappy("_: sub(2 1 1);", Float.unwrap(LibDecimalFloat.packLossless(0, -75)), "2 1 1");
+        checkHappy("_: sub(2 1 1);", 0, "2 1 1");

75-100: Optional: add property-style zero-difference fuzz tests

Adding invariants like sub(x, x, 0…0) == 0 and sub(x, 0…0) == x would nicely complement these exact cases and catch regressions in varargs handling and zero normalization.

I can draft two compact property-style tests that reuse existing helpers and compare via Float.unwrap(LibDecimalFloat.packLossless(...)) where needed. Want me to push a snippet?

src/lib/op/math/LibOpCeil.sol (2)

4-4: Consolidate duplicate imports from the same interface file


16-16: Fix grammar in comment: "one inputs" → "one input"

test/src/lib/op/math/LibOpCeil.t.sol (1)

32-43: Add missing edge-case tests for ceil

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 65e3d81 and 294bd0f.

📒 Files selected for processing (6)
  • src/lib/op/math/LibOpCeil.sol (1 hunks)
  • test/src/concrete/RainterpreterReferenceExtern.intInc.t.sol (2 hunks)
  • test/src/lib/op/math/LibOpAdd.t.sol (2 hunks)
  • test/src/lib/op/math/LibOpCeil.t.sol (1 hunks)
  • test/src/lib/op/math/LibOpExp.t.sol (1 hunks)
  • test/src/lib/op/math/LibOpSub.t.sol (2 hunks)
🔇 Additional comments (8)
test/src/lib/op/math/LibOpSub.t.sol (3)

45-45: Modernized harness signature looks good

Switching to external view with StackItem[] aligns with the OperandV2/StackItem patterns elsewhere. No issues spotted.


57-59: Reference-check path is aligned with other ops

Using opReferenceCheck with opTestDefaultInterpreterState is consistent with the new testing approach and provides a solid reference-vs-runtime validation.


86-87: Updated expectations reflect new decimal-float scaling

The -67 exponent expectations for sub(1 2) and sub(1 0.1) line up with the migration to the new decimal-float encoding. Looks correct.

test/src/concrete/RainterpreterReferenceExtern.intInc.t.sol (1)

43-44: Test expectations updated to match new decimal float encoding scheme

The updated test values now use packLossless(4e66, -66) and packLossless(3e66, -66) instead of the previous packLossless(4e37, -37) and packLossless(3e37, -37). These still represent the values 4 and 3 respectively, just with a different mantissa/exponent representation that aligns with the PR-wide shift to the new decimal float encoding scheme.

Also applies to: 61-62

test/src/lib/op/math/LibOpExp.t.sol (1)

44-44: Test expectations correctly updated for new decimal float representation

The expected values for exp(1), exp(0.5), exp(2), and exp(3) have been updated to use mantissas in scientific notation with adjusted exponents (-66, -65). These changes preserve the same mathematical values while aligning with the new decimal float encoding scheme used throughout the PR.

Also applies to: 49-49, 54-54, 59-59

src/lib/op/math/LibOpCeil.sol (1)

10-47: Migration to decimal floating point completed successfully

The implementation correctly migrates from UD60x18 fixed-point to decimal floating point using LibDecimalFloat. The integrity, runtime, and reference functions all properly handle the new OperandV2 and StackItem types. The assembly blocks are correctly marked as "memory-safe" and the ceil operation is appropriately delegated to the library.

test/src/lib/op/math/LibOpCeil.t.sol (2)

10-29: Test migration to new types completed successfully

The test correctly uses the new OperandV2, Float, LibDecimalFloat, and StackItem types. The integrity test properly validates the 1-input, 1-output constraint, and the runtime test appropriately uses the opReferenceCheck helper with the updated type signatures.


34-34: Verify consistency in expected output values

The expected outputs appear inconsistent with the ceil operation:

  • Line 34: ceil(0.5) should return 1, but the expected value is packLossless(1e67, -67) which represents 0.1
  • Line 36: ceil(2.5) should return 3, but the expected value is packLossless(3e66, -66) which represents 0.3

These expected values don't match the mathematical ceil operation. Please verify whether these are the correct expected outputs or if there's a discrepancy in the test expectations.

Also applies to: 36-36, 37-37

Comment thread test/src/lib/op/math/LibOpAdd.t.sol
Comment thread test/src/lib/op/math/LibOpAdd.t.sol
Comment thread test/src/lib/op/math/LibOpAdd.t.sol
Comment thread test/src/lib/op/math/LibOpSub.t.sol
Comment thread test/src/lib/op/math/LibOpSub.t.sol

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🔭 Outside diff range comments (1)
src/lib/op/math/LibOpHeadroom.sol (1)

11-13: Grammar and clarity: “a decimal floating-point number” and integer semantics

Tighten the wording and document the integer case explicitly.

Apply this diff to fix the grammar and clarify semantics:

-/// @notice Opcode for the headroom (distance to ceil) of an decimal floating
-/// point number.
+/// @notice Opcode for the headroom (distance to ceil) of a decimal floating-point
+/// number. For integer inputs, headroom returns 1 (not 0).
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 294bd0f and c541098.

⛔ Files ignored due to path filters (1)
  • src/generated/Rainterpreter.pointers.sol is excluded by !**/generated/**
📒 Files selected for processing (2)
  • src/lib/op/math/LibOpHeadroom.sol (2 hunks)
  • test/src/lib/op/math/LibOpHeadroom.t.sol (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-artifacts)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-static)
  • GitHub Check: rainix (macos-latest, rainix-rs-test)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-static)
  • GitHub Check: rainix (ubuntu-latest, rainix-sol-artifacts)
  • GitHub Check: rainix (ubuntu-latest, test-wasm-build)
  • GitHub Check: rainix (ubuntu-latest, rainix-rs-test)
  • GitHub Check: rainix (macos-latest, rainix-rs-artifacts)
  • GitHub Check: git-clean
🔇 Additional comments (3)
src/lib/op/math/LibOpHeadroom.sol (1)

29-32: LGTM: correct headroom via ceil(x) - x with integer clamp

The move to compute headroom as a.ceil().sub(a) and clamp zero to one matches the intended (0,1] semantics and handles negatives correctly.

test/src/lib/op/math/LibOpHeadroom.t.sol (2)

34-34: LGTM: updated decimal-float encodings

The packLossless values for 0.5 and 0.2 (from headroom(3.8)) match the new 10^67 scaling. Expected outputs remain correct.

Also applies to: 37-37


39-43: Great addition: negative-input coverage

Adding cases for -1, -0.5, -2, -3, and -3.8 locks in the intended behavior for negatives; results align with ceil(x) - x and the integer clamp.

Comment thread src/lib/op/math/LibOpHeadroom.sol
Comment thread test/src/lib/op/math/LibOpHeadroom.t.sol

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
test/src/lib/op/math/LibOpAdd.t.sol (2)

102-110: 2-input expectations align with new decimal-float normalization; add a brief inline rationale for -66/-67

These cases look correct and consistent with packLossless semantics. Consider a short comment to anchor readers on why -66/-67 exponents are chosen as the canonical normalization for small integers.

Apply this diff to add the note:

     function testOpAddEval2InputsHappy() external view {
+        // Note: Expected encodings use canonical normalization for small integers
+        // under the new decimal-float scheme (e.g., 11 -> 11e66 @ -66, 1 -> 1e67 @ -67).
         checkHappy("_: add(5 6);", Float.unwrap(LibDecimalFloat.packLossless(11e66, -66)), "5 + 6");
         checkHappy("_: add(6 5);", Float.unwrap(LibDecimalFloat.packLossless(11e66, -66)), "6 + 5");
 
         checkHappy("_: add(-5 -6);", Float.unwrap(LibDecimalFloat.packLossless(-11e66, -66)), "-5 + -6");
         checkHappy("_: add(-6 -5);", Float.unwrap(LibDecimalFloat.packLossless(-11e66, -66)), "-6 + -5");
 
         checkHappy("_: add(-5 6);", Float.unwrap(LibDecimalFloat.packLossless(1e67, -67)), "-5 + 6");
         checkHappy("_: add(6 -5);", Float.unwrap(LibDecimalFloat.packLossless(1e67, -67)), "6 + -5");
     }

160-195: DRY the repeated overflow sentinels with named constants; consider adding a 2-input overflow case

The overflow sentinel literals are repeated across all unhappy-path checks. Centralizing them improves readability and eases future updates if encoding changes again. Prior feedback suggested the same.

Define the sentinels once (outside this range, near the top of the file, before the contract):

// Overflow sentinels for decimal-float packing under add.
// COEFFICIENT: sentinel coefficient observed on overflow path.
// EXPONENT: 2^31, intentionally outside int32 range to flag exponent overflow.
int256 constant OVERFLOW_COEFFICIENT_SENTINEL =
    2695994666715063979466701508701963067363714442254057248110361024921;
int256 constant OVERFLOW_EXPONENT_SENTINEL = 2147483648;

Then use them here:

         checkUnhappyOverflow(
             "_: add(max-positive-value() max-positive-value() 1e-18);",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );
         checkUnhappyOverflow(
             "_: add(max-positive-value() 1e-18 max-positive-value());",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );
         checkUnhappyOverflow(
             "_: add(1e-18 max-positive-value() max-positive-value());",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );
         checkUnhappyOverflow(
             "_: add(max-positive-value() max-positive-value() max-positive-value());",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );
         checkUnhappyOverflow(
-            "_: add(max-positive-value() max-positive-value() 0);",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            "_: add(max-positive-value() max-positive-value() 0);",
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );
         checkUnhappyOverflow(
-            "_: add(max-positive-value() 0 max-positive-value());",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            "_: add(max-positive-value() 0 max-positive-value());",
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );
         checkUnhappyOverflow(
-            "_: add(0 max-positive-value() max-positive-value());",
-            2695994666715063979466701508701963067363714442254057248110361024921,
-            2147483648
+            "_: add(0 max-positive-value() max-positive-value());",
+            OVERFLOW_COEFFICIENT_SENTINEL,
+            OVERFLOW_EXPONENT_SENTINEL
         );

Optional: Re-introduce a 2-input overflow unhappy-path case to ensure coverage symmetry with the 3-input path.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c541098 and 6bc106a.

📒 Files selected for processing (1)
  • test/src/lib/op/math/LibOpAdd.t.sol (2 hunks)
🔇 Additional comments (1)
test/src/lib/op/math/LibOpAdd.t.sol (1)

149-153: LGTM: 3-input permutations and normalization look correct

The 3-input happy-path expectations (18e65 @ -65) are consistent and provide good permutation coverage.

Comment thread test/src/lib/op/math/LibOpAdd.t.sol
Comment thread test/src/lib/op/math/LibOpAdd.t.sol
@thedavidmeister
thedavidmeister merged commit 0813336 into main Aug 12, 2025
10 of 11 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Sep 18, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant